Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

docs(tutorial): Fix location of filename introduction #586

Merged
merged 3 commits into from
Apr 28, 2024
Merged

docs(tutorial): Fix location of filename introduction #586

merged 3 commits into from
Apr 28, 2024

Conversation

infinisil
Copy link
Contributor

@infinisil infinisil commented Apr 15, 2024 •
edited
Loading

Description

Previously the filename option was only mentioned in the Python-specific section. This moves it earlier to the correct place.

Checklist

  • Make sure all GitHub Actions checks are green (they are automatically checking all of the above).
  • Ensure docs/tutorial.rst is still up-to-date.

@infinisil infinisil requested a review from a team as a code owner April 15, 2024 16:21
@adiroiban
Copy link
Member

Thanks for the change.

The result is here https://towncrier--586.org.readthedocs.build/en/586/tutorial.html

adiroiban
adiroiban reviewed Apr 15, 2024
View reviewed changes
docs/tutorial.rst Outdated
@@ -19,6 +19,9 @@ The most basic configuration is just telling ``towncrier`` where to look for new

[tool.towncrier]
directory = "changes"
# Where you want your news files to come out. This can be .rst
# or .md, towncrier's default template works with both.
filename = "NEWS.rst"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The most basic configuration is just telling towncrier where to look for news fragments::

Now we also need to specify the filename ?

-The most basic configuration is just telling ``towncrier`` where to look for news fragments::
+The most basic configuration is just telling ``towncrier`` where to look for news fragments, and it will generate a `NEWS.rst` file in the project root::

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I now changed it to

The most basic configuration is just telling towncrier where to look for news fragments and what file to generate::

to keep the same style of only saying what it does, without giving the value of the option (which comes in the later paragraph).

adiroiban
adiroiban requested changes Apr 16, 2024
View reviewed changes
Copy link
Member

@adiroiban adiroiban left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks for the contribution. Welcome abord!

I am not sure I understand this change.

I think that the purpose of the first paragraph is to get the project working with minimal configuration.

From what I can see in the documentation

https://towncrier.readthedocs.io/en/latest/configuration.html

filename is not a required argument and for now it defaults to NEWS.rst

I would prefer to leave the default filename option.

In the future, we might add another configuration option to specify if RST or MD should be used, and then we can default to something like NEWS.md

@infinisil
Copy link
Contributor Author

Currently, the tutorial contains this part:

The most basic configuration is just telling towncrier where to look for news fragments:

[tool.towncrier]
directory = "changes"

Which will look into “./changes” for news fragments and write them into “./NEWS.rst”.

Note the and write them into “./NEWS.rst”..

This confused me when reading, because while I clearly saw how ./changes was specified, it's not clear why ./NEWS.rst would be used.

Only after reading the next paragraph the connection can be made, but that section is about Python specifically:

If you’re working on a Python project, you can also specify a package:

[tool.towncrier]
# The name of your Python package
package = "myproject"
# The path to your Python package.
# If your package lives in 'src/myproject/', it must be 'src',
# but if you don't keep your code in a 'src' dir, remove the
# config option
package_dir = "src"
# Where you want your news files to come out. This can be .rst
# or .md, towncrier's default template works with both.
filename = "NEWS.rst"

But there it's very easy to miss.

filename is not a required argument and for now it defaults to NEWS.rst

Ah I didn't know that. I think it's still worth pointing out that it can be specified to avoid the above confusion, maybe something like this:

[tool.towncrier]
directory = "changes"
# Where you want your news files to come out, `NEWS.rst` is the default.
# This can be .rst or .md, towncrier's default template works with both.
# filename = "NEWS.rst"

@adiroiban
Copy link
Member

Many thanks for your feedback.

The feedback from people new to the project is critical.

Ah I didn't know that. I think it's still worth pointing out that it can be specified to avoid the above confusion, maybe something like this:

Agree.

Beside changing the configuration example, I think that is important to also update the narrative doc paragraph from above that introduces the example.

I left an inline comment for that.

Thanks

adiroiban
adiroiban approved these changes Apr 18, 2024
View reviewed changes
Copy link
Member

@adiroiban adiroiban left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. This PR is missing a newsfragment :p

Please add the file and then this should be ready to merge.

Thanks a lot!

@infinisil
docs(tutorial): Improve location of filename introduction
82a33e2 
Currently, the tutorial contains this part:

> The most basic configuration is just telling towncrier where to look for news fragments:
>
> ```toml
> [tool.towncrier]
> directory = "changes"
> ```
>
> Which will look into “./changes” for news fragments and write them into “./NEWS.rst”.

Note the `and write them into “./NEWS.rst”.`.

This confused me when reading, because while I clearly saw how `./changes` was specified, it's not clear why `./NEWS.rst` would be used.

Only after reading the next paragraph the connection can be made, but that section is about Python specifically:

> If you’re working on a Python project, you can also specify a package:
>
> ```toml
> [tool.towncrier]
> # The name of your Python package
> package = "myproject"
> # The path to your Python package.
> # If your package lives in 'src/myproject/', it must be 'src',
> # but if you don't keep your code in a 'src' dir, remove the
> # config option
> package_dir = "src"
> # Where you want your news files to come out. This can be .rst
> # or .md, towncrier's default template works with both.
> filename = "NEWS.rst"
> ```

But there it's very easy to miss.

This commit moves the introduction of the filename option to the earlier
section to avoid such confusion. Furthermore we indicate that there's
no need to set the option because there's a default.
@adiroiban adiroiban enabled auto-merge (squash) April 28, 2024 12:12
@adiroiban
Copy link
Member

Thanks for the update. I have enabled auto-merge. I hope this will get merged soon.

@adiroiban adiroiban merged commit 90b75b4 into twisted:trunk Apr 28, 2024
15 checks passed
@infinisil infinisil deleted the tutorial-file-introduction branch April 28, 2024 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adiroiban adiroiban adiroiban approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@infinisil @adiroiban